fix(parser): preserve TrackedSet on plural 'those tokens' delayed exile#6199
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
There was a problem hiding this comment.
Code Review
This pull request resolves issue #5972 by ensuring that plural 'those tokens' references, which are rewritten to TrackedSet, are not incorrectly overwritten by LastCreated in rewrite_parent_target_to_last_created. The match criteria in lower.rs have been updated to exclude TargetFilter::TrackedSet, and the twinflame_full_parse test has been expanded to assert that the delayed trigger's zone change correctly targets the TrackedSet. No review comments were provided, so there is no additional feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the parser change identifies the plural anaphor but sends battlefield tokens to an exile-only delayed-trigger resolver.
🔴 Blockers
-
crates/engine/src/parser/oracle_effect/lower.rs:3032-3045preservesTrackedSetfor the cleanup, butcrates/engine/src/game/effects/delayed_trigger.rs:687-737then turns every delayedChangeZone { TrackedSet }intoChangeZoneAll { origin: Exile }. The current parse artifact already shows Mysterio's cleanup origin changingbattlefield → ∅. Saheeli, Mysterio, and Twinflame create the referenced tokens on the battlefield, so this resolver cannot exile them. Move the completion semantics to a zone/provenance-aware tracked-set binding atbind_tracked_set_to_effect, and add a production delayed-trigger regression that creates multiple tokens, resolves the cleanup, and proves a token that left the battlefield first is not found. -
The current head fails
parser::oracle::tests::altair_ibn_la_ahad_for_each_exile_memory_counter_copy_parsesatcrates/engine/src/parser/oracle_tests.rs:800: Altaïr's printed “Exile those tokens at end of combat” now lowers toTrackedSet, while the stale test requiresLastCreated. Update this sibling through the same runtime design; do not restore the single-token binding. -
The current
<!-- coverage-parse-diff -->reports only Mysterio, while the PR claims Saheeli.parse_effect_chainshape assertions do not prove the actual card pipeline or resolution. Reconcile the card-level diff and add the runtime proof for the claimed card class.
✅ Clean
- Scryfall confirms the relevant plural cleanup wording for Saheeli, Mysterio, and Altaïr. The parse-diff is current, and the existing Gemini review raised no separate technical finding.
Recommendation: request changes; move the completion semantics into the delayed-trigger tracked-set binding seam, then cover the multiple-token and left-battlefield cases through the real resolution pipeline.
|
Maintainer fixup pushed to
The refreshed CI suite is now the verification authority. |
|
Hi, @matthewevans |
|
Acknowledged. The maintainer fixup is still the current head, but its current CI run is red, so this remains blocked pending the failing-check diagnosis and a green rerun. |
de8a5fa to
903d268
Compare
|
Maintainer fixup pushed to The failing assertion required Saheeli in the deliberately curated integration fixture, but that fixture contains Twinflame only. I removed the invalid duplicate card-data assertion; the parser-shape test and the real Twinflame delayed-cleanup regression remain. CI has restarted for the new head. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved after the maintainer fixups and branch update. The delayed-trigger binding now preserves the battlefield-origin tracked set, the remaining runtime regression exercises two Twinflame tokens through end-step cleanup, and the invalid fixture assertion was removed. Required CI is still running; merge queue will wait for it.
Review feedback (phase-rs#6274): the non-vacuity floors shipped at 2845/1480 sat exactly at the PRE-refresh catalog, so reverting this PR's regen still passed every assertion — the guard could not detect losing the refresh it was added alongside. The pre-PR test asserted the parent counts as equalities (assert_eq!(total_tokens, 2845)) and was green on main, so the parent catalog measures exactly those floors. Ratchet all floors to the committed vintage (2858/1490) and add the third axis already carried on the summary, source_card_refs >= 9821. `>=` still keeps weekly upstream ADDITIONS green — the false-red phase-rs#6237 introduced — while any shrink now fails. The ref floor is the load-bearing one and is deliberately tight: across the ten recorded revisions of known-tokens.toml, tokens and rules_text are monotone, and source_card_refs shrank exactly once — phase-rs#6199, a parser PR that silently dropped 1173 token<->card links (9810 -> 8637) while the token count GREW past a count-based floor. This regen is what repaired it. Each floor is probed in isolation, since sequential assert!s let an earlier conjunct dominate a later one and render it vacuous: - catalog reverted to f0ac543^ -> FAIL "token catalog shrank: 2845 presets < 2858" - 1 rules_text line removed -> FAIL rules_text_tokens >= 1490 - 5 source_card_refs blocks removed -> FAIL source_card_refs >= 9821 Catalog restored byte-identically after each probe. Floors carry the measured count in the panic so a CI red names the axis and the delta instead of requiring a local repro. Assisted-by: ClaudeCode:claude-opus-4.8
Review feedback (phase-rs#6274): the non-vacuity floors shipped at 2845/1480 sat exactly at the PRE-refresh catalog, so reverting this PR's regen still passed every assertion — the guard could not detect losing the refresh it was added alongside. The pre-PR test asserted the parent counts as equalities (assert_eq!(total_tokens, 2845)) and was green on main, so the parent catalog measures exactly those floors. Ratchet all floors to the committed vintage (2858/1490) and add the third axis already carried on the summary, source_card_refs >= 9821. `>=` still keeps weekly upstream ADDITIONS green — the false-red phase-rs#6237 introduced — while any shrink now fails. The ref floor is the load-bearing one and is deliberately tight: across the ten recorded revisions of known-tokens.toml, tokens and rules_text are monotone, and source_card_refs shrank exactly once — phase-rs#6199, a parser PR that silently dropped 1173 token<->card links (9810 -> 8637) while the token count GREW past a count-based floor. This regen is what repaired it. Each floor is probed in isolation, since sequential assert!s let an earlier conjunct dominate a later one and render it vacuous: - catalog reverted to f0ac543^ -> FAIL "token catalog shrank: 2845 presets < 2858" - 1 rules_text line removed -> FAIL rules_text_tokens >= 1490 - 5 source_card_refs blocks removed -> FAIL source_card_refs >= 9821 Catalog restored byte-identically after each probe. Floors carry the measured count in the panic so a CI red names the axis and the delta instead of requiring a local repro. Assisted-by: ClaudeCode:claude-opus-4.8
…hot vintage-proof (#6274) * fix(engine): commit W30 MTGJSON token catalog and make coverage snapshot vintage-proof The weekly MTGJSON refresh introduced by #6237 (vintage gate 2026-07-20) unfroze a stale token catalog cache. Deterministic regen (byte-identical across two checkouts) against the 2026-07-20 MTGJSON data adds 13 tokens (12 Secret Lair Drop printings: Food x8, Treasure, Ooze, Elf Warrior x2; 1 HOB Goblin Army) and 1184 [[token.source_card_refs]] (8637 -> 9821), and picks up MTGJSON's face-name change 'Undercity // The Initiative' -> 'Undercity'. No tokens removed; coverage stays complete (2858/2858 supported, 1490/1490 rules-text tokens parsed). analyze_token_coverage_treats_source_defined_pt_as_represented pinned vintage-dependent absolutes (2845/1480), which go red every week upstream adds tokens even at 100% coverage. Replace them with invariants (supported == total, parsed == rules_text) plus non-vacuity floors at the last-known-good baseline so an empty or truncated catalog cannot pass vacuously. Assisted-by: ClaudeCode:claude-fable-5 * test(engine): ratchet token-catalog floors to the committed vintage Review feedback (#6274): the non-vacuity floors shipped at 2845/1480 sat exactly at the PRE-refresh catalog, so reverting this PR's regen still passed every assertion — the guard could not detect losing the refresh it was added alongside. The pre-PR test asserted the parent counts as equalities (assert_eq!(total_tokens, 2845)) and was green on main, so the parent catalog measures exactly those floors. Ratchet all floors to the committed vintage (2858/1490) and add the third axis already carried on the summary, source_card_refs >= 9821. `>=` still keeps weekly upstream ADDITIONS green — the false-red #6237 introduced — while any shrink now fails. The ref floor is the load-bearing one and is deliberately tight: across the ten recorded revisions of known-tokens.toml, tokens and rules_text are monotone, and source_card_refs shrank exactly once — #6199, a parser PR that silently dropped 1173 token<->card links (9810 -> 8637) while the token count GREW past a count-based floor. This regen is what repaired it. Each floor is probed in isolation, since sequential assert!s let an earlier conjunct dominate a later one and render it vacuous: - catalog reverted to f0ac543^ -> FAIL "token catalog shrank: 2845 presets < 2858" - 1 rules_text line removed -> FAIL rules_text_tokens >= 1490 - 5 source_card_refs blocks removed -> FAIL source_card_refs >= 9821 Catalog restored byte-identically after each probe. Floors carry the measured count in the panic so a CI red names the axis and the delta instead of requiring a local repro. Assisted-by: ClaudeCode:claude-opus-4.8 * docs(engine): document the provenance seam for the token-catalog floors The floors observe totals; provenance (committed catalog == tokens-gen output for the declared MTGJSON vintage) is structurally untestable at test time (the ~560 MB gitignored generator input is absent; the test sees build.rs's embed of the tracked file) and is verified at the generation seam instead (gen-card-data.sh temp+cmp+vintage gate). Comment documents the seam and the by-hand reproduction recipe, per review 4746023659. Assisted-by: ClaudeCode:claude-opus-4.8
Closes #5972
Summary
Discord report: Saheeli, the Gifted's [-7] creates a token copy of each artifact you control, grants them haste, then should exile all of those tokens at the next end step — but only the last-created token was exiled.
Root cause: after
rewrite_parent_targets_to_tracked_setcorrectly bound plural "those tokens" toTrackedSet { id: 0 }withuses_tracked_set: true, the post-tokenresolve_populated_token_anaphorspass rewroteTrackedSet→LastCreatedon the delayedChangeZoneexile.LastCreatedonly snapshots the final token in a multi-token batch.Fix: preserve
TrackedSetinrewrite_parent_target_to_last_created'sChangeZonearm. Singular "it"/"that token" cleanup (Flameshadow Conjuring, Inalla) still rewritesParentTarget/TriggeringSource→LastCreated.Changes
crates/engine/src/parser/oracle_effect/lower.rs— stop stompingTrackedSettoLastCreatedon delayed plural-token exile cleanup.crates/engine/src/parser/oracle_effect/tests.rs— Saheeli -7 regression: delayed exile staysTrackedSet+uses_tracked_set: true.crates/engine/src/parser/oracle_tests.rs— Twinflame regression: inner delayed exile target isTrackedSet, notLastCreated.Test Plan
cargo fmt --all -- --checkcargo test -p engine --lib saheeli_minus_seven_those_tokens_delayed_exile_keeps_tracked_setcargo test -p engine --lib twinflame_full_parsecargo test -p engine --lib etb_token_copier_exile_anaphor_binds_created_tokencargo test -p engine --lib delayed_trigger_change_zone_after_token_creator_rewrites_to_last_created./scripts/gen-card-data.shor Tiltcard-data) so Saheeli's [-7] execute body picks up the parser fix in runtime